Conversation
|
cc @pepijnve since you're working on these same files |
There was a problem hiding this comment.
🟡 Changes recommended
Same-name pass-through aliases remain excluded from deduplication and can recreate the ambiguous schema.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes projection merging to resolve pass-through columns against the input schema.
Changes:
- Normalizes column qualifiers before deduplication.
- Adds an SQL regression test for the planning failure.
File summaries
| File | Description |
|---|---|
extract_leaf_expressions.rs |
Resolves merged pass-through columns against input schemas. |
struct.slt |
Tests the previously failing union query. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25412 +/- ##
========================================
Coverage 82.48% 82.48%
========================================
Files 1140 1140
Lines 437341 437679 +338
Branches 437341 437679 +338
========================================
+ Hits 360721 361031 +310
- Misses 54835 54846 +11
- Partials 21785 21802 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…xtraction projection
`build_extraction_projection_impl` merges an extraction projection into the
projection below it, and adds the pass-through columns that the merged
projection does not already carry. It compared the columns it was about to add
against the projection's own expressions without putting the two in the same
space.
A projection can list bare column names over a qualified input. Eliminating the
empty side of a union leaves exactly that. The comparison then misses, the
column is added a second time under its bare name, and `Projection::try_new`
rejects the result:
Optimizer rule 'push_down_leaf_projections' failed
caused by
Schema error: Schema contains qualified field name samples.env and
unqualified field name env which would be ambiguous
Resolve both sides against the input schema, and push the column under the name
the input gives it.
…ion projection A projection can spell a pass-through column as `t.c AS c`. The merge only counted bare column expressions as existing pass-throughs, so it added `t.c` beside the `c` the alias already outputs, which is an ambiguous schema. Use `passthrough_column` to collect them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0bc684b to
61923e2
Compare
adriangb
left a comment
There was a problem hiding this comment.
Summary
The fix is correct and small. I verified all claims in the description. I have 3 non-blocking suggestions (inline).
Root cause
flowchart TD
A["Filter / Aggregate above<br/>needs: <code>env</code> (bare) or <code>samples.env</code>"] --> M{{"build_extraction_projection_impl<br/>merge into existing Projection"}}
P["Existing Projection: <code>v, s, env</code> (bare)<br/>input: <code>samples.v, samples.s, samples.env</code>"] --> M
M -->|main: compare raw Column values| X["<code>samples.env</code> != <code>env</code><br/>→ push again<br/>→ schema has <code>samples.env</code> AND <code>env</code><br/>❌ ambiguous"]
M -->|PR: resolve both sides against input| O["both → <code>samples.env</code><br/>→ match, no push<br/>✅ plans"]
Verification
| Check | Result |
|---|---|
cargo test -p datafusion-optimizer --lib extract_leaf_expressions on PR head (61923e2) |
✅ 57 passed |
| Same, with only the fix reverted (tests kept) | ❌ 2 failed: both new tests |
sqllogictests -- struct projection_pushdown on PR head |
✅ pass |
Simplified resolve_against (suggestion 1) |
✅ 57 passed |
Behavior change table
| Needed column | Input schema | main |
This PR |
|---|---|---|---|
bare c |
t.c |
pushes bare c |
pushes t.c (input spelling) |
t.c |
t.c, existing has bare c |
pushes t.c → ambiguous |
matched, no push |
t.c |
t.c, existing has t.c AS c |
pushes t.c → ambiguous |
matched, no push |
| any | column not in input (for example __common_expr_N) |
skipped | skipped (unchanged) |
Suggestions
| # | Where | Kind | Blocking |
|---|---|---|---|
| 1 | resolve_against |
Simplify to one lookup; fix doc claim about duplicate names | No |
| 2 | merge comment | Make shorter | No |
| 3 | struct.slt |
Add EXPLAIN to lock the plan shape |
No |
The doc on resolve_against says a duplicate name "resolves to nothing". That is not always true. qualified_field_with_unqualified_name returns the one unqualified field when the name is also held by qualified fields (see dfschema.rs). The behavior is fine; only the doc is wrong. Suggestion 1 fixes both.
- Resolve both qualified and unqualified columns with one `qualified_field_from_column` lookup. The result always carries the input schema's qualifier, so two spellings of the same column compare equal. Correct the doc: a duplicate name held by exactly one unqualified field resolves to that field, not to `None`. - Make the merge comment shorter. - Add a logical EXPLAIN to the struct.slt case, so the test shows when union elimination stops producing the shape it covers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@pepijnve if you are able to review this it would be great, we are hitting this in prod so pretty keen to fix it. I also think it's a good starting place to tackle the EPIC we have now. |
| } | ||
| } | ||
|
|
||
| /// The way `schema` names `col`, or `None` when `schema` does not hold it |
There was a problem hiding this comment.
nit: this sentence took me a couple of tries to grok. The function has an active name (an action), but the first sentence is describing a noun (a property). Might be better to write this in a 'Does computation xyz' style.
| // by alias expressions (e.g., CSE's __common_expr_N) exist in the output but | ||
| // not the input, and cannot be added as pass-through Column references. | ||
| // | ||
| // Compare both sides in the input's spelling (see `resolve_against`). |
There was a problem hiding this comment.
What are the 'sides' being referred to here? In the final sentence there's a reference to an empty union side. Makes this hard to interpret.
Which issue does this PR close?
None filed.
Rationale for this change
This query fails to plan on
main:enable_leaf_expression_pushdownis on by default, so this is a plain planningfailure for a valid statement. A
union allwhose one side is provably empty iswhat produces the shape, and query generators emit that: a dashboard panel adds a
second branch behind a comparison of two constants and the panel stops working.
What changes are included in this PR?
build_extraction_projection_implmerges an extraction projection into theprojection below it, and then adds the pass-through columns the merged projection
does not already carry. It compared the columns it was about to add against the
projection's own expressions without putting the two in the same space.
A projection can list bare column names over a qualified input. Removing the empty
side of the union leaves exactly that. The comparison then misses, the column is
added a second time under its bare name, and
Projection::try_newrejects a schemathat holds
samples.envand a bareenvtogether.This PR resolves both sides against the input schema before the comparison, and
pushes the column under the name the input gives it. A name the input schema holds
more than once resolves to nothing, because no single spelling is correct there.
A same-name alias such as
t.c AS ccounts as a pass-through on the existing side too, the same as a baret.c.Are these changes tested?
Yes. The statement above is added to
struct.slt. It fails onmainwith theerror above and passes with this change.
Two unit tests in
extract_leaf_expressions.rscover the merge directly. Both fail onmainwith an ambiguoustest.aerror:test_merge_bare_column_into_qualified_projection: a filter namesaover a projection that outputstest.a.test_merge_into_projection_with_same_name_alias: the projection spells the pass-through astest.a AS a.cargo test --workspace --exclude datafusion-sqllogictestpasses: 126 suites,12479 tests, 0 failures. The sqllogictest suite passes too: 520 files, 0
failures.
cargo clippy -p datafusion-optimizer --all-targetsandcargo fmt --all -- --checkare clean.Are there any user-facing changes?
A statement of this shape plans instead of failing. No API change.
Note on overlapping work
#25388 (draft) edits the same arm of
build_extraction_projection_impl, for a different problem: duplicatedevaluation of
KeepInPlaceexpressions, #25329.It does not touch the qualifier comparison this PR changes, so the two are
independent in behaviour, but whichever lands second will need a small rebase at
the tail of the
columns_neededloop.#25282 also appends to the end of
struct.slt, as this PR does. Keeping both blocks is the whole resolution.Part of the leaf-pushdown EPIC: #25459